Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add suspicious group #7350

Merged
merged 3 commits into from
Jun 28, 2021
Merged

Add suspicious group #7350

merged 3 commits into from
Jun 28, 2021

Conversation

camsteffen
Copy link
Contributor

@camsteffen camsteffen commented Jun 14, 2021 •
edited
Loading

changelog: Introduce clippy::suspicious 🤔 group and move several lints into the group

Closes #6366. CC #6626.

A number of lints are moved from each of correctness, style and complexity groups. Notably I didn't move suspicious_splitn since I think that is a correctness lint despite the name.

Lints moved to clippy::suspicious:

  • blanket_clippy_restriction_lints (was clippy::style)
  • empty_loop (was clippy::style)
  • eval_order_dependence (was clippy::complexity)
  • float_equality_without_abs (was clippy::correctness)
  • for_loops_over_fallibles (was clippy::correctness)
  • misrefactored_assign_op (was clippy::complexity)
  • mut_range_bound (was clippy::complexity)
  • mutable_key_type (was clippy::correctness)
  • suspicious_arithmetic_impl (was clippy::correctness)
  • suspicious_assignment_formatting (was clippy::style)
  • suspicious_else_formatting (was clippy::style)
  • suspicious_map (was clippy::complexity)
  • suspicious_op_assign_impl (was clippy::correctness)
  • suspicious_unary_op_formatting (was clippy::style)

@rust-highfive
Copy link

r? @giraffate

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jun 14, 2021
@flip1995 flip1995 added the I-nominated Issue: Nominated to be discussed at the next Clippy meeting label Jun 15, 2021
@flip1995
Copy link
Member

Let's talk about this in our meeting later today, before we finalize this.

@flip1995
Copy link
Member

r? @flip1995 (reassigning, since @giraffate takes a short break)

@rust-highfive rust-highfive assigned flip1995 and unassigned giraffate Jun 15, 2021
xFrednet
xFrednet reviewed Jun 15, 2021
View reviewed changes
Copy link
Member

@xFrednet xFrednet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good to me. I focused on the addition in the metadata collection monster and that looks correct. It's nice to see that it's as simple as planned :)

@xFrednet xFrednet mentioned this pull request Jun 15, 2021
Merged
@flip1995
Copy link
Member

In today's meeting there was no veto to going forward with this. As Manish pointed out, I'll go through the Clippy RFC to see if something prevents us to do this, but we can probably do this.

flip1995
flip1995 reviewed Jun 16, 2021
View reviewed changes
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading the RFC, there's nothing preventing us from adding lint groups. Though, we can't really remove a group after adding it.

I still have to look at the lints separately.

README.md Outdated Show resolved Hide resolved
@llogiq
Copy link
Contributor

llogiq commented Jun 17, 2021

I like it! Not sure if any lints are missing from the list, but those who are there seem to be about right.

flip1995
flip1995 approved these changes Jun 25, 2021
View reviewed changes
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for taking so long to review. The lints that were moved LGTM. One typo in the readme is left, otherwise r=me (after squashing the fixups).

README.md Outdated Show resolved Hide resolved
@flip1995 flip1995 added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed I-nominated Issue: Nominated to be discussed at the next Clippy meeting S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Jun 25, 2021
@flip1995
Copy link
Member

flip1995 commented Jun 25, 2021 •
edited
Loading

Oh, it would also be great, if you could edit the PR description to include from which group a lint was moved for each lint, so that writing the changelog for this will be easier :)

@camsteffen
Copy link
Contributor Author

Added remark-gfm in order to allow the table on the readme to exceed the maximum width. See remarkjs/remark-lint#249 (comment). Needed this after updating the description of clippy::all to mention suspicious.

@camsteffen
Copy link
Contributor Author

Oh, it would also be great, if you could edit the PR description to include from which group a lint was moved for each lint, so that writing the changelog for this will be easier :)

Done! I'll let you review one more time for the remark change.

@flip1995
Copy link
Member

Remark changes also LGTM.

Thanks! @bors r+

@bors
Copy link
Contributor

bors commented Jun 28, 2021

📌 Commit 5b5f0ea has been approved by flip1995

@bors
Copy link
Contributor

bors commented Jun 28, 2021

⌛ Testing commit 5b5f0ea with merge e405c68...

@bors
Copy link
Contributor

bors commented Jun 28, 2021

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing e405c68 to master...

@bors bors merged commit e405c68 into rust-lang:master Jun 28, 2021
@camsteffen camsteffen deleted the suspicious branch July 8, 2021 22:01
@diwic diwic mentioned this pull request Jul 12, 2021
Merged
@flip1995 flip1995 mentioned this pull request Jul 28, 2021
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xFrednet xFrednet xFrednet left review comments

@flip1995 flip1995 flip1995 approved these changes

Assignees

@flip1995 flip1995

Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Missing lint categories?
7 participants
@camsteffen @rust-highfive @flip1995 @llogiq @bors @xFrednet @giraffate